RoundRobinLoadBalancer reduce copy/resize for connection add/remove #1514
Conversation
| } else { | ||
| Object[] newList = new Object[existing.length - 1]; | ||
| System.arraycopy(existing, 0, newList, 0, i); | ||
| System.arraycopy(existing, i + 1, newList, i, newList.length - i); |
There was a problem hiding this comment.
Consider adding a case for i == 0, then you can do a single arraycopy:
System.arraycopy(existing, 1, newList, 0, newList.length);There was a problem hiding this comment.
I'm not sure it is worth it bcz arraycopy already does bounds checking. we could also specialize other cases (e.g. i == existing.length - 1) but lets save for followup investigation.
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Show resolved
Hide resolved
| } | ||
| } | ||
| } else { | ||
| for (int i = 0; i < LARGE_LIST_SELECTION_ATTEMPTS; i++) { |
There was a problem hiding this comment.
Can we keep the previous logic in this case: (int) (connections.length * 0.75f)? 64 attempts looks low if users have 1000s of connections
There was a problem hiding this comment.
I will remove the selection algorithm part of this PR and move to another PR.
| int collisions = 0; | ||
| final int collisionThreshold = connections.length >>> 1; | ||
| while (i < connections.length) { | ||
| int index = rnd.nextInt(connections.length); |
There was a problem hiding this comment.
This algorithm is a bit involved and may have collisions. In the worst case it iterates over all connections. Consider taking a random value only for the first test and then continue in a loop over all connections.
There was a problem hiding this comment.
I will experiment with this, and I'll move the selection changes to another PR.
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Show resolved
Hide resolved
Motivation: RoundRobinLoadBalancer's Host class maintains a list of active connections in a copy-on-write fashion. However when an item is added or removed their is an additional copy operation due to initial state not accounting for 1 more/less element. Modifications: - Host class uses an array for copy-on-write storage and allocates exactly the required amount of space on add/remove. Result: Less resize/copy operations on connection established/closed.
Scottmitch
force-pushed
the
rr_selection
branch
from
64d292b to
eb19c43
Compare
Scottmitch
changed the title
| } else { | ||
| Object[] newList = new Object[existing.length - 1]; | ||
| System.arraycopy(existing, 0, newList, 0, i); | ||
| System.arraycopy(existing, i + 1, newList, i, newList.length - i); |
|
|
||
| final Addr address; | ||
| private volatile List<C> connections = emptyList(); | ||
| private volatile Object[] connections = EMPTY_ARRAY; |
There was a problem hiding this comment.
This could be ListenableAsyncCloseable
| for (;;) { | ||
| List<C> existing = this.connections; | ||
| if (existing == CLOSED_LIST) { | ||
| final Object[] existing = this.connections; | ||
| if (existing == CLOSED_ARRAY) { | ||
| return false; | ||
| } | ||
| ArrayList<C> connectionAdded = new ArrayList<>(existing); | ||
| connectionAdded.add(connection); | ||
| if (connectionsUpdater.compareAndSet(this, existing, connectionAdded)) { | ||
| Object[] newList = Arrays.copyOf(existing, existing.length + 1); | ||
| newList[existing.length] = connection; | ||
| if (connectionsUpdater.compareAndSet(this, existing, newList)) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
do {} while(cU.compareAndSet()) would avoid the break
| connection.onClose().beforeFinally(() -> { | ||
| for (;;) { | ||
| final List<C> existing = connections; | ||
| if (existing == CLOSED_LIST) { | ||
| final Object[] existing = this.connections; | ||
| if (existing == CLOSED_ARRAY) { | ||
| break; | ||
| } | ||
| ArrayList<C> connectionRemoved = new ArrayList<>(existing); | ||
| if (!connectionRemoved.remove(connection) || | ||
| connectionsUpdater.compareAndSet(this, existing, connectionRemoved)) { | ||
| int i = 0; | ||
| for (; i < existing.length; ++i) { | ||
| if (existing[i].equals(connection)) { | ||
| break; | ||
| } | ||
| } | ||
| if (i == existing.length) { | ||
| break; | ||
| } else { | ||
| Object[] newList = new Object[existing.length - 1]; | ||
| System.arraycopy(existing, 0, newList, 0, i); | ||
| System.arraycopy(existing, i + 1, newList, i, newList.length - i); | ||
| if (connectionsUpdater.compareAndSet(this, existing, newList)) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| }).subscribe(); |
There was a problem hiding this comment.
Perhaps make this a standalone removeConnectionMethod() and call that in beforeFinally?
| if (existing == CLOSED_ARRAY) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Not really necessary as CLOSED_ARRAY is always empty.
|
|
||
| final Addr address; | ||
| private volatile List<C> connections = emptyList(); | ||
| private volatile Object[] connections = EMPTY_ARRAY; |
There was a problem hiding this comment.
How big is connections likely to be and how much contention is there likely to be? Could perhaps just use or extend CopyOnWriteArraySet
There was a problem hiding this comment.
Its internal CopyOnWriteArrayList uses ReentrantLock that we prefer to avoid as it may content on the event-loop threads.
…1514) Motivation: RoundRobinLoadBalancer's Host class maintains a list of active connections in a copy-on-write fashion. However when an item is added or removed their is an additional copy operation due to initial state not accounting for 1 more/less element. Modifications: - Host class uses an array for copy-on-write storage and allocates exactly the required amount of space on add/remove. Result: Less resize/copy operations on connection established/closed.
Motivation:
RoundRobinLoadBalancer's Host class maintains a list of active
connections in a copy-on-write fashion. However when an item is added or
removed their is an additional copy operation due to initial state not
accounting for 1 more/less element.
Modifications:
exactly the required amount of space on add/remove.
Result:
Less resize/copy operations on connection established/closed.